Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rootViewControllerを足してloginとlogoutの遷移処理を書いた #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shinya-todaka
Copy link
Contributor

概要

  • マージはしないで下さい
  • rootViewControllerを追加した

レビュー観点

  • 参考 https://github.com/sgr-ksmt/Shopping-Cart/blob/master/iOS/Shopping-Cart/Sources/RootViewController.swift
  • 遷移はRootViewController.instance.transition(to: .hoge)でする
  • logoutの処理が抜けてますがサインアウトした後にRootViewController.instance.transition(to: .login)でしています。
  • RootViewControllerのinstanceをとるのに
    UIApplication.shared.keyWindow!.rootViewController as! RootViewControllerとしているのがちょっと不安
    windowのrootViewControllerをRootViewControllerにしてmakeKeyAndVisibleしないことはないと思うから多分大丈夫?

レビューレベル

  • Lv0: まったく見ないでAcceptする
  • Lv1: ぱっとみて違和感がないかチェックしてAcceptする
  • Lv2: 仕様レベルまで理解して、仕様通りに動くかある程度検証してAcceptする
  • Lv3: 実際に環境で動作確認したうえでAcceptする

スクリーンショット

rootViewController

備考

@shinya-todaka shinya-todaka requested review from ojun9 and ren-suke July 13, 2020 12:58
@shinya-todaka
Copy link
Contributor Author

これkeyWindowはdeprecatedになってたので使っちゃダメでした。

extension UIViewController {

  var rootViewController: RootViewController? {
    var controller: UIViewController? = self

    while controller?.parent != nil {
      if let containerController = controller?.parent as? RootViewController {
        return containerController
      }
      controller = controller?.parent
    }
    return nil
  }
}

使うとしたらこんな感じのextensionを書いた方が良いと思います。
参考 https://qiita.com/mitsuyoshi/items/93682c4d8ce0e4096bf6

Copy link
Contributor

@ojun9 ojun9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

みました!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants